Improve token stability by making the API token a "unique" index. #40
Conversation
Centralized code that performs token updates (removed duplicate code). Implemented a test to assert token uniqueness.
| $tokenData = $this->generateToken( $expired ); | ||
|
|
||
| // ensure we never regenerate the same token! | ||
| if($owner->{$tokenDBColumn} != $tokenData['token']){ |
There was a problem hiding this comment.
The point where the token would collide is in the Database. So, if we add the type check, do we improve or worsen the stability of the check?
The only distinction on the DB side would be 'null' != null? Otherwise everything should be treated as string (varchar). I think it's sensible to assume both tokens will become strings, so I'd stick with the != check, since checking for type inequality isn't important to the DB... or am I missing something important here?
There was a problem hiding this comment.
tokenData should always be a string, so it should be fine to use !== no?
There was a problem hiding this comment.
Yeah, I guess stricter is better here. So using !== is fine.
|
This looks good I think. Only had 2 small questions. Even if fixing edge cases, it's a great security addition. Thx. |
|
This code has issues with MsSQL DBs. Closing this request. |
|
OK.. What was the MsSQL issue? Also, I do think we should make sure tokens are unique somehow. UUID seem the best solution. |
|
The current implementation of SilverStripe MsSQL bindings creates unique indexes that won't allow multiple Thus, declaring the |
Centralized code that performs token updates (removed duplicate code).
Implemented a test to assert token uniqueness.
See Issue #39